Enable TFListener to get Authority #545
Conversation
Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
|
CI (build:
|
|
|
||
| TEST(transform_listener, authority_present) | ||
| { | ||
| const std::string pub_namespace = "/tflistnertest/ns"; |
There was a problem hiding this comment.
| const std::string pub_namespace = "/tflistnertest/ns"; | |
| const std::string pub_namespace = "/tflistenertest/ns"; |
| auto listener = tf2_ros::TransformListener(buffer, listener_node); | ||
|
|
||
| // Wait for pub/sub to match | ||
| while(pub->get_subscription_count() == 0) { |
There was a problem hiding this comment.
| while(pub->get_subscription_count() == 0) { | |
| while (pub->get_subscription_count() == 0) { |
Also, can we put a timeout on this? In case this fails for some reason, I'd prefer this didn't hang forever.
| pub->publish(msg); | ||
|
|
||
| // Wait for sub to get message | ||
| while(!buffer.canTransform("parent", "child", tf2::TimePointZero)) { |
There was a problem hiding this comment.
| while(!buffer.canTransform("parent", "child", tf2::TimePointZero)) { | |
| while (!buffer.canTransform("parent", "child", tf2::TimePointZero)) { |
Same comment here about adding a timeout.
| const std::string as_yaml = buffer.allFramesAsYAML(); | ||
| EXPECT_TRUE(as_yaml.find(pub_node_name) != std::string::npos) << as_yaml; | ||
| EXPECT_TRUE(as_yaml.find(pub_namespace) != std::string::npos) << as_yaml; |
There was a problem hiding this comment.
I know that this is the easiest way to do this, but it seems imprecise. That is, we aren't really checking that the authority was set properly; we're just checking whether the node name exists anywhere in that YAML, which could happen for other reasons. Is there a way we could make these checks more precise?
| for (const rclcpp::TopicEndpointInfo & pub : endpoints) { | ||
| // Compare gid to see if this is the right publisher | ||
| bool match = true; | ||
| for (decltype(RMW_GID_STORAGE_SIZE) i = 0; i < RMW_GID_STORAGE_SIZE; ++i) { |
There was a problem hiding this comment.
I'm somewhat concerned about the performance impact of this check, particularly on networks with lots of /tf publishers. While I know that the inner loop has a fixed number of iterations, it is still the case that we are going to be doing this linear search on every single reception of a tf2 frame, which seems expensive. Should we consider a map or an unordered_map here?
There was a problem hiding this comment.
@clalancette you mean, keeping an updated map GID -> EndpointInfo, in order to simplify this part of code by retrieving the node by key GID?
std::unordered_map would be a good choice? What are the numerical properties of Node GIDs? Are there known specific/custom hashing strategies used somewhere else to take into account?
I'm trying to figure out what this one needs to get merged. Other comments seem easy fixes.
| _make_authority_str( | ||
| const std::vector<rclcpp::TopicEndpointInfo> & endpoints, | ||
| const rmw_gid_t & pub_gid, | ||
| std::string & authority) |
There was a problem hiding this comment.
This is a nit, but instead of an output authority variable, maybe we can have this function return std::string, with an empty string meaning "not found"? I'm not sure if that API is better, so feel free to disregard this comment if you don't like it.
|
@sloretz What is the status of this PR? Do you think you might pick it back up, or should we close it for now? |
This enables the
TFListenerto get the authority, that is the name of the node containing the publisher who published the transform.This doesn't enable
tf2_monitorto get the authority. I didn't see a good API to get the data there, so I left it unresolved.